Skip to content

feat: TypeScript rewrite with zero dependencies, interface-based DI, and comprehensive tests#61

Open
joalves wants to merge 22 commits intomainfrom
worktree-typescript-rewrite
Open

feat: TypeScript rewrite with zero dependencies, interface-based DI, and comprehensive tests#61
joalves wants to merge 22 commits intomainfrom
worktree-typescript-rewrite

Conversation

@joalves
Copy link
Copy Markdown
Contributor

@joalves joalves commented Apr 18, 2026

Summary

Complete TypeScript rewrite of the JavaScript SDK with zero runtime dependencies, strict type safety, and 424 tests.

  • Zero runtime dependencies — removes core-js, node-fetch, rfdc; custom MD5/Murmur3 implementations
  • Interface-based dependency injectionClient, ContextDataProvider, ContextPublisher interfaces with Default* implementations
  • Domain error hierarchyABSmartlyError, ContextNotReadyError, ContextFinalizedError for typed error handling
  • Modern build tooling — tsup (replaces Babel + Webpack) producing ESM, CJS, IIFE, and ES2015 legacy IIFE builds
  • Strict TypeScriptnoUncheckedIndexedAccess, noUnusedLocals, noUnusedParameters, noImplicitReturns
  • Bug fixes from original SDK audit_flush snapshot-and-restore, audience null→mismatch safety default, surrogate pair hashing, _evaluateAudience try/catch, _init config parse guard
  • Input validation on all public methods matching original SDK behavior
  • New APIsreadyError(), getSDK(), getOptions(), generic variableValue<T>(), custom field access
  • Optional polyfill injectionfetchImpl/AbortControllerImpl for Node.js 14-17 support
  • 424 tests (was ~200 in original) covering timers, flush resilience, logger safety, operator evaluation, audience matching, and all public API validation

Also published to absmartly/typescript-sdk.

Test plan

  • npx tsc --noEmit — strict TypeScript compile check
  • npm test — 424 tests pass
  • npm run build — ESM, CJS, IIFE, legacy IIFE all build
  • Verify cross-SDK assignment compatibility with deterministic test vectors
  • Integration test against A/B Smartly sandbox API
  • Browser smoke test with dist/index.global.js

Summary by CodeRabbit

Release Notes

  • New Features

    • Added named exports for SDK and Context classes; improved TypeScript support throughout the SDK
    • Introduced custom field accessor methods and enhanced error classes for better error handling
    • Added legacy browser bundle (dist/index.legacy.js) for ES2015 environments
  • Breaking Changes

    • Major version bump (v2.0.0); updated import statements from default to named exports
    • Dropped Internet Explorer 10 support; requires Node.js 14+ (18+ recommended)
    • Removed bundled AbortController export; use platform/global implementation
    • Renamed provider/publisher classes; see migration guide in documentation
  • Documentation

    • Comprehensive migration guide from v1 with updated API examples and bundle filenames

joalves added 22 commits March 28, 2026 16:57
…E legacy claims, document removed AbortController export
… comprehensive tests

Merge improvements from both TypeScript SDK rewrites into a single
codebase, fixing critical bugs and adding 118 new tests (424 total).

Types & Architecture:
- Split types.ts into models.ts (wire-format) and interfaces.ts (DI contracts)
- Add Client, ContextDataProvider, ContextPublisher interfaces with Default* implementations
- Add domain error hierarchy: ABSmartlyError, ContextNotReadyError, ContextFinalizedError
- Stronger Assignment type: required fields with null instead of optional
- Add ExperimentVariant, ExperimentApplication, GoalAchievement named types
- Add readyError(), getSDK(), getOptions() public API methods
- Add input validation on all public methods matching original SDK

Critical bug fixes:
- _flush: snapshot-and-restore pattern prevents data loss on publish failure
- Audience mismatch: null evaluation → true (safe exclude), matching original SDK
- _evaluateAudience: try/catch wrapper prevents evaluator crashes
- stringToUint8Array: handle UTF-16 surrogate pairs (emoji/4-byte UTF-8)
- _init: isObject() guard on parsed config + error logging
- Client: forward per-request timeout to retry logic

Important fixes:
- getUnits() returns defensive copy
- _getAttributesMap() caches result until attrs change
- _flush logs discard when context is failed
- _logEvent/_logError wrapped in try/catch (logger can't crash SDK)
- _customFieldValue uses _logError instead of console.error
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 18, 2026

Walkthrough

This pull request represents a comprehensive TypeScript rewrite of the ABSmartly JavaScript SDK (v1.13.4 → v2.0.0). The changes include migrating the build system from webpack/Babel to tsup, replacing Jest with Vitest, eliminating legacy browser shims, refactoring the public API from mixed default/named exports to explicit named exports, renaming core provider/publisher classes to use Default prefixes, introducing strict TypeScript typing across all modules, consolidating scattered JSON expression operators into a unified module, and updating all test suites to Vitest syntax. Configuration files for ESLint, Prettier, and Babel are removed in favour of TypeScript-based tooling.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related PRs

Suggested reviewers

  • mario-silva
  • marcio-absmartly

Poem

🐰 From Jest to Vitest, the tests now align,
TypeScript strict types make the SDK shine,
Webpack and Babel fade into the past,
Tsup builds bundles faster and vast,
Named exports blossom, v2 takes flight,
Hop forward with types that feel just right! 🚀

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch worktree-typescript-rewrite
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch worktree-typescript-rewrite

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/md5.ts (1)

152-153: ⚠️ Potential issue | 🟡 Minor

Set the high 32 bits of MD5 bit-length padding.

Line 152 sets only the low word. For payloads larger than 2^29 bytes, the digest becomes incorrect because the high length word is omitted.

🐛 Suggested fix
 	block[14] = l << 3;
+	block[15] = l >>> 29;
 	md5cycle(state, block);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/md5.ts` around lines 152 - 153, The padding sets only the low 32-bit word
(block[14] = l << 3) so very large payloads lose the high 32 bits of the
bit-length; update the padding to assign both block[14] and block[15] so the
full 64-bit bit-length is passed into md5cycle — compute the low 32 bits as (l
<< 3) and the high 32 bits from the upper bits (e.g., combine (l >>> 29) with h
<< 3 or equivalent using the existing length high variable) and set block[15]
accordingly before calling md5cycle(state, block).
src/config.ts (1)

45-66: ⚠️ Potential issue | 🟡 Minor

Missing path segment creation for intermediate levels.

When traversing a path like a.b.c and a exists but a.b does not, the code at line 45 checks if (frag in target) but only handles the case where the property exists. For intermediate segments (when index < frags.length - 1), missing properties should be created as empty objects to allow the final getter to be defined.

🐛 Proposed fix to create missing intermediate objects
 			if (frag in target) {
 				if (index < frags.length - 1) {
 					if (!isObject(target[frag])) {
 						console.warn(
 							`Config key '${variableKey}' for experiment '${experimentName}' is overriding non-object value at '${frags.slice(0, index + 1).join(".")}' with an object.`,
 						);
 						target[frag] = {};
 						target = target[frag] as Record<string, unknown>;
 					} else {
 						target = target[frag] as Record<string, unknown>;
 					}
 				}
+			} else if (index < frags.length - 1) {
+				target[frag] = {};
+				target = target[frag] as Record<string, unknown>;
 			}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/config.ts` around lines 45 - 66, The code only handles existing path
segments but skips creating missing intermediate objects; update the traversal
in the block around target/frag/frags/index so that when index < frags.length -
1 and frag is not present on target you create target[frag] = {} (then set
target = target[frag] as Record<string, unknown>), while keeping the existing
branch that warns and replaces non-objects; leave the Object.defineProperty
logic for the final segment (using experimentName,
context.variableValue(variableKey, defaultValue), and the _{frag}_setter)
unchanged.
src/context.ts (1)

857-887: ⚠️ Potential issue | 🔴 Critical

Don't let finalize() race with an in-flight auto-flush.

After the snapshot is taken, _pending is set to 0 before publish() settles. That lets finalize() observe no pending work and resolve while the publish is still in flight; if the request then fails, the catch path restores events into an already-finalised context and they can no longer be retried. Please track the in-flight flush promise/flag and make _finalize() await it before sealing the context.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/context.ts` around lines 857 - 887, The publish snapshot code can race
with finalize(): record the in-flight publish as a property (e.g.,
this._inFlightFlush or this._isFlushing + this._inFlightFlushPromise) when you
call this._publisher.publish(request,...), set it before awaiting/then/catch,
and clear it in both the then and catch handlers; update the catch to still
restore pendingCount/pendingExposures/pendingGoals as it does now. Modify
finalize() (or _finalize()) to await this in-flight flush promise (or check the
_isFlushing flag) before sealing the context so a concurrent publish failure can
restore events and be retried. Ensure the new property is initialized on the
context class and cleared in all publish completion paths.
🧹 Nitpick comments (10)
src/__tests__/provider.test.ts (1)

12-13: Add a call-count assertion for stronger behavioural guarantees.

Consider asserting a single invocation (toHaveBeenCalledTimes(1)) to catch accidental duplicate getContext() calls.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/provider.test.ts` around lines 12 - 13, Add an assertion that
mockGetContext was invoked exactly once to prevent accidental duplicate calls;
update the test in provider.test.ts to include
expect(mockGetContext).toHaveBeenCalledTimes(1) alongside the existing
expect(mockGetContext).toHaveBeenCalledWith({ path: "/test" }) and
expect(result).toEqual({ experiments: [] }) so the test verifies both the call
arguments and the single invocation of getContext (mockGetContext).
src/errors.ts (1)

1-20: Consider unifying all SDK errors under ABSmartlyError.

Now that ABSmartlyError exists, keeping TimeoutError, RetryError, and AbortError on plain Error limits unified error handling (instanceof ABSmartlyError) for consumers.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/errors.ts` around lines 1 - 20, Change the SDK error hierarchy so all
specific errors extend ABSmartlyError instead of Error: update TimeoutError,
RetryError, AbortError (and any other plain Error subclasses) to subclass
ABSmartlyError, ensure their constructors call super(message) and set this.name
(e.g., "TimeoutError", "RetryError", "AbortError"); keep existing
ABSmartlyError, ContextNotReadyError, and ContextFinalizedError behavior but
ensure their prototypes align with ABSmartlyError so consumers can use
instanceof ABSmartlyError for unified error handling.
src/__tests__/md5.test.ts (1)

4-44: Harden UTF-8 test encoding for surrogate pairs.

Line 4’s helper does not encode surrogate pairs, so future non-BMP vectors would be encoded incorrectly. Given the surrogate-pair bugfix objective, this is worth locking down in the tests.

♻️ Suggested update
 function stringToBuffer(value: string): ArrayBuffer {
-	const n = value.length;
-	const array: number[] = [];
-	let k = 0;
-	for (let i = 0; i < n; ++i) {
-		const c = value.charCodeAt(i);
-		if (c < 0x80) {
-			array[k++] = c;
-		} else if (c < 0x800) {
-			array[k++] = (c >> 6) | 192;
-			array[k++] = (c & 63) | 128;
-		} else {
-			array[k++] = (c >> 12) | 224;
-			array[k++] = ((c >> 6) & 63) | 128;
-			array[k++] = (c & 63) | 128;
-		}
-	}
-	return Uint8Array.from(array).buffer;
+	const bytes: number[] = [];
+	for (let i = 0; i < value.length; i++) {
+		let c = value.charCodeAt(i);
+		if (c >= 0xd800 && c <= 0xdbff && i + 1 < value.length) {
+			const next = value.charCodeAt(i + 1);
+			if (next >= 0xdc00 && next <= 0xdfff) {
+				c = ((c - 0xd800) << 10) + (next - 0xdc00) + 0x10000;
+				i++;
+			}
+		}
+		if (c < 0x80) {
+			bytes.push(c);
+		} else if (c < 0x800) {
+			bytes.push((c >> 6) | 0xc0, (c & 0x3f) | 0x80);
+		} else if (c < 0x10000) {
+			bytes.push((c >> 12) | 0xe0, ((c >> 6) & 0x3f) | 0x80, (c & 0x3f) | 0x80);
+		} else {
+			bytes.push((c >> 18) | 0xf0, ((c >> 12) & 0x3f) | 0x80, ((c >> 6) & 0x3f) | 0x80, (c & 0x3f) | 0x80);
+		}
+	}
+	return Uint8Array.from(bytes).buffer;
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/md5.test.ts` around lines 4 - 44, The stringToBuffer helper
does not handle UTF-16 surrogate pairs (non-BMP characters) and will misencode
code points above 0xFFFF; update stringToBuffer to iterate by Unicode code
points (using codePointAt and advancing i by 1 or 2 appropriately) or use
TextEncoder to produce a UTF-8 ArrayBuffer, ensuring md5 tests feed correct
UTF-8 bytes; locate and change the stringToBuffer function and keep toHex and
the test cases unchanged.
src/utils.ts (1)

107-109: Clever but potentially confusing short-circuit logic.

The arrayEqualsShallow function uses double negation and optional chaining that may be hard to follow. Consider adding a brief comment explaining the logic.

📝 Consider adding a clarifying comment
+// Returns true if both arrays are identical references, both null/undefined,
+// or have the same length with all elements strictly equal
 export function arrayEqualsShallow(a?: unknown[] | null, b?: unknown[] | null): boolean {
 	return a === b || (a?.length === b?.length && !a?.some((va, vi) => b && va !== b[vi]));
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/utils.ts` around lines 107 - 109, The function arrayEqualsShallow uses a
compact short-circuit: it returns true if both references are identical,
otherwise it checks equal lengths and that no element differs via
a?.some((va,vi)=> b && va !== b[vi]); add a brief clarifying comment above
arrayEqualsShallow that explains these steps (reference identity check, length
equality, shallow strict-equality element comparison) and calls out the optional
chaining/guard on b to avoid null access so readers understand the intent and
safety.
package.json (1)

49-55: Consider pinning devDependency versions for reproducible builds.

The devDependencies use caret ranges (e.g., ^8.0.0, ^5.4.0), which allows minor/patch updates. For a major SDK release, consider using exact versions or narrower ranges to ensure reproducible builds across different environments.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@package.json` around lines 49 - 55, Update package.json devDependencies to
use pinned exact versions instead of caret ranges: replace entries under the
"devDependencies" object (tsup, typescript, vitest, tsx, `@vitest/coverage-v8`) to
fixed version strings (e.g., "tsup": "8.0.0") to ensure reproducible builds;
make the change where the "devDependencies" key and those package names are
defined so CI and local installs always pull the exact same versions.
src/index.ts (1)

35-47: Consider exporting DefaultClient for advanced use cases.

The DefaultClient class is not exported, which means consumers cannot instantiate it directly or extend it. If advanced users need to create custom client configurations outside of SDK, consider adding it to the exports.

export { DefaultClient } from "./client";

Alternatively, if this is intentional to keep the API surface minimal, this is acceptable as users can still inject a custom Client implementation via SDKOptions.client.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/index.ts` around lines 35 - 47, The public API currently re-exports types
from "./interfaces" but does not export the DefaultClient class, preventing
advanced users from instantiating or extending it; add an export for
DefaultClient from "./client" (e.g., export { DefaultClient } from "./client")
so consumers can directly import DefaultClient while leaving existing
SDKOptions.client injection unchanged and reference DefaultClient in
documentation/examples; locate the existing export block in this file to add the
new export alongside the type exports.
src/interfaces.ts (1)

67-74: Consider typing context parameter more specifically.

The ContextPublisher.publish method uses unknown for the context parameter. If Context is the expected type, consider using a more specific type or a minimal interface to improve type safety for implementers.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/interfaces.ts` around lines 67 - 74, The ContextPublisher.publish
signature currently types the context parameter as unknown; change it to a more
specific type (e.g., Context or a minimal interface like PublishContext) to
improve type safety: update the publish method signature in the ContextPublisher
interface to use that concrete type for the context parameter, import or declare
the Context/PublishContext type in this file, and then update all
implementations of ContextPublisher.publish to accept and use the new typed
context (adjust any call sites or tests accordingly).
src/client.ts (2)

119-125: Redundant null check on response text.

response.text() always resolves to a string (possibly empty), so the text !== null check on line 121 is unnecessary. Consider simplifying to just check for empty string.

♻️ Suggested simplification
 				return response.text().then((text: string) => {
 					const error: Error & { _bail?: boolean } = new Error(
-						text !== null && text.length > 0 ? text : response.statusText,
+						text.length > 0 ? text : response.statusText,
 					);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/client.ts` around lines 119 - 125, The null check on the text from
response.text() is redundant; update the error construction inside the
response.text().then(...) callback (the block that creates const error: Error &
{ _bail?: boolean }) to test for empty string only (e.g., use text.length > 0 or
text !== "" to select text, otherwise use response.statusText), then set
error._bail = bail and reject as before.

152-158: Original error stack trace lost when wrapping errors.

Lines 153-154 create new Error instances that discard the original error's stack trace. Consider preserving the cause using the cause option (ES2022+) or attaching the original error.

♻️ Proposed fix to preserve error cause
 			return tryOnce().catch((reason: Error & { _bail?: boolean }) => {
-				if (reason._bail || retries <= 0) throw new Error(reason.message);
+				if (reason._bail || retries <= 0) throw new Error(reason.message, { cause: reason });
 				if (tries >= retries) throw new RetryError(tries, reason, url);

Note: The RetryError constructor already accepts the original error, which is good. Consider the same pattern for the bail case.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/client.ts` around lines 152 - 158, The code is losing the original error
stack by creating new Error instances in the catch of tryOnce; update the bail
and timeout/error branches to preserve the original error as the cause (or
rethrow the original error) instead of throwing plain new Error(reason.message).
Specifically, in the catch handling inside tryOnce's promise you should: for the
bail case use throw new Error("bail: ...", { cause: reason }) or rethrow reason;
for the timeout/AbortError branch ensure you throw TimeoutError or the original
reason with cause preserved (TimeoutError should include the original reason as
its cause if appropriate). Keep references to reason, retries, tries,
RetryError, TimeoutError and tryWith.timedout when making these changes so the
original stack is preserved.
src/jsonexpr/evaluator.ts (1)

178-181: Loose equality check on boolean comparison result.

Line 180 uses != instead of !== when checking rvalue. Since booleanConvert always returns a boolean (never undefined), this check will always pass. Consider using !== for consistency with the rest of the codebase, or simply remove the check since rvalue is always truthy or false.

♻️ Suggested simplification
 			case "boolean": {
 				const rvalue = this.booleanConvert(rhs);
-				if (rvalue != null) return lhs === rvalue ? 0 : lhs > rvalue ? 1 : -1;
-				break;
+				return lhs === rvalue ? 0 : lhs > rvalue ? 1 : -1;
 			}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/jsonexpr/evaluator.ts` around lines 178 - 181, The boolean branch in the
evaluator uses a loose inequality when checking rvalue from
this.booleanConvert(rhs); update the case "boolean" logic to either use strict
comparison (rvalue !== null) or, better, remove the null check entirely since
booleanConvert always returns a boolean; directly compute and return lhs ===
rvalue ? 0 : lhs > rvalue ? 1 : -1 in the case "boolean" block (identify and
modify the code around this.booleanConvert and the local rvalue variable).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@README.md`:
- Around line 181-188: The example in README.md uses an invalid script type
causing the block to be inert; update the <script> tag around the
createContextWith(...) example to either omit the type attribute or use a valid
MIME type such as "text/javascript" (or "application/javascript") so the
JavaScript that calls sdk.createContextWith(...) will execute in the browser.

In `@scripts/generate-version.ts`:
- Around line 3-4: Guard the package.json version before writing src/version.ts:
check that the parsed pkg.version exists and is a string (e.g., validate
pkg.version !== undefined && typeof pkg.version === "string") in the
generate-version script, and if the check fails, print a clear error and exit
non‑zero instead of calling writeFileSync; when valid, write the SDK_VERSION as
before. Ensure you reference pkg.version, the writeFileSync call that writes
`export const SDK_VERSION = ...`, and fail fast with a descriptive message to
avoid emitting an invalid constant.

In `@src/__tests__/client.test.ts`:
- Around line 90-96: Several test suites stub the global fetch via
vi.stubGlobal("fetch", vi.fn()) in their beforeEach hooks but only call
vi.restoreAllMocks() in afterEach; add vi.unstubAllGlobals() to each afterEach
that pairs with a vi.stubGlobal call (the afterEach blocks adjacent to the
beforeEach that stubs fetch — look for the vi.stubGlobal("fetch", vi.fn()) calls
in src/__tests__/client.test.ts) so globals are removed between tests; ensure
you call vi.unstubAllGlobals() after vi.restoreAllMocks() in each affected
afterEach (the blocks around lines shown in the review: the afterEach following
the beforeEach that stubs fetch at ~lines 90–96, 125–131, and 174–182).

In `@src/__tests__/errors.test.ts`:
- Around line 61-66: Update the "has correct name and default message" test for
AbortError to actually assert the default message: after creating const error =
new AbortError(); add an assertion that checks error.message equals the
AbortError default (e.g., expect(error.message).toBe(AbortError.DEFAULT_MESSAGE)
or, if there is no static constant, assert the known default string used by the
class, or at minimum expect(typeof error.message).toBe("string") and
expect(error.message.length).toBeGreaterThan(0); this ensures the default
message is verified alongside the existing checks for instance and name.

In `@src/__tests__/sdk.test.ts`:
- Around line 54-62: The test stubs the global fetch via vi.stubGlobal(...) but
only calls vi.restoreAllMocks(), so the global stub remains; update the test to
call vi.unstubAllGlobals() after restoring mocks (or instead of) to fully remove
the global stub — locate the SDK instantiation and context creation in the test
(SDK, createContext) and add a vi.unstubAllGlobals() call (e.g., after
vi.restoreAllMocks() or replace it) to properly clean up the global fetch stub.

In `@src/hashing.ts`:
- Around line 7-13: The surrogate-handling logic allows lone UTF-16 surrogates
to be encoded directly; update the blocks that inspect c, next, i and
value.charCodeAt(...) so that any unpaired high surrogate (0xD800–0xDBFF with no
valid following low surrogate) or any lone low surrogate (0xDC00–0xDFFF) is
replaced with the Unicode replacement code point 0xFFFD before proceeding to
UTF-8 byte emission; specifically, in the branch that currently computes a
paired surrogate (where you set c = ((c - 0xd800) << 10) + (next - 0xdc00) +
0x10000 and increment i), keep that for valid pairs, but when the next char is
absent or not in 0xDC00–0xDFFF set c = 0xFFFD (and do not increment i), and in
the other branch that sees a low surrogate alone also set c = 0xFFFD — ensure
these changes are applied to the same surrogate-checking sections so subsequent
UTF-8 encoding uses 0xFFFD for lone surrogates.

In `@src/interfaces.ts`:
- Around line 15-17: ContextParams currently only exposes units, but the
codebase supports initial attributes via the Attribute interface and
PublishParams; add an optional attributes?: Attribute[] to the ContextParams
type so callers can supply initial attributes on creation, update any
constructors or factory functions that accept ContextParams (e.g., context
creation methods) to read and apply params.attributes when present, and ensure
typings referencing ContextParams (and any usage in PublishParams flows) accept
and propagate the new optional field.

In `@src/jsonexpr/operators.ts`:
- Around line 31-37: The OrCombinator.evaluate implementation currently treats
an empty args array as truthy by returning args.length === 0; change this so
that an empty disjunction returns false instead. Locate the
OrCombinator.evaluate method in src/jsonexpr/operators.ts and replace the final
return that checks args.length with a straight false (so { or: [] } evaluates to
false) while keeping the existing loop and
evaluator.booleanConvert(evaluator.evaluate(expr)) logic intact.

In `@src/sdk.ts`:
- Around line 139-150: The _validateParams method currently assumes params.units
exists and iterates it, causing a TypeError when callers like createContext or
createContextWith pass no units; update _validateParams to explicitly handle a
missing or falsy params.units (e.g., treat undefined/null as an empty object or
return early) before the for-loop so validation only runs when units is present,
keeping the existing checks for type and empty strings intact and referencing
_validateParams, createContext, and createContextWith to locate the related call
sites.
- Around line 60-72: The constructor currently typed as constructor(options:
ClientOptions & SDKOptions) forces required ClientOptions even when
options.client is provided; change the signature to a union that allows
providing a custom client without supplying DefaultClient fields, e.g.
constructor(options: (ClientOptions & SDKOptions) | (Partial<ClientOptions> &
SDKOptions & { client: ClientInterface })), leaving the internal logic using
options.client and new DefaultClient(clientOptions) unchanged; update any
related type imports (ClientOptions, SDKOptions, ClientInterface) and ensure
this._client assignment code still compiles against the new union type.

---

Outside diff comments:
In `@src/config.ts`:
- Around line 45-66: The code only handles existing path segments but skips
creating missing intermediate objects; update the traversal in the block around
target/frag/frags/index so that when index < frags.length - 1 and frag is not
present on target you create target[frag] = {} (then set target = target[frag]
as Record<string, unknown>), while keeping the existing branch that warns and
replaces non-objects; leave the Object.defineProperty logic for the final
segment (using experimentName, context.variableValue(variableKey, defaultValue),
and the _{frag}_setter) unchanged.

In `@src/context.ts`:
- Around line 857-887: The publish snapshot code can race with finalize():
record the in-flight publish as a property (e.g., this._inFlightFlush or
this._isFlushing + this._inFlightFlushPromise) when you call
this._publisher.publish(request,...), set it before awaiting/then/catch, and
clear it in both the then and catch handlers; update the catch to still restore
pendingCount/pendingExposures/pendingGoals as it does now. Modify finalize() (or
_finalize()) to await this in-flight flush promise (or check the _isFlushing
flag) before sealing the context so a concurrent publish failure can restore
events and be retried. Ensure the new property is initialized on the context
class and cleared in all publish completion paths.

In `@src/md5.ts`:
- Around line 152-153: The padding sets only the low 32-bit word (block[14] = l
<< 3) so very large payloads lose the high 32 bits of the bit-length; update the
padding to assign both block[14] and block[15] so the full 64-bit bit-length is
passed into md5cycle — compute the low 32 bits as (l << 3) and the high 32 bits
from the upper bits (e.g., combine (l >>> 29) with h << 3 or equivalent using
the existing length high variable) and set block[15] accordingly before calling
md5cycle(state, block).

---

Nitpick comments:
In `@package.json`:
- Around line 49-55: Update package.json devDependencies to use pinned exact
versions instead of caret ranges: replace entries under the "devDependencies"
object (tsup, typescript, vitest, tsx, `@vitest/coverage-v8`) to fixed version
strings (e.g., "tsup": "8.0.0") to ensure reproducible builds; make the change
where the "devDependencies" key and those package names are defined so CI and
local installs always pull the exact same versions.

In `@src/__tests__/md5.test.ts`:
- Around line 4-44: The stringToBuffer helper does not handle UTF-16 surrogate
pairs (non-BMP characters) and will misencode code points above 0xFFFF; update
stringToBuffer to iterate by Unicode code points (using codePointAt and
advancing i by 1 or 2 appropriately) or use TextEncoder to produce a UTF-8
ArrayBuffer, ensuring md5 tests feed correct UTF-8 bytes; locate and change the
stringToBuffer function and keep toHex and the test cases unchanged.

In `@src/__tests__/provider.test.ts`:
- Around line 12-13: Add an assertion that mockGetContext was invoked exactly
once to prevent accidental duplicate calls; update the test in provider.test.ts
to include expect(mockGetContext).toHaveBeenCalledTimes(1) alongside the
existing expect(mockGetContext).toHaveBeenCalledWith({ path: "/test" }) and
expect(result).toEqual({ experiments: [] }) so the test verifies both the call
arguments and the single invocation of getContext (mockGetContext).

In `@src/client.ts`:
- Around line 119-125: The null check on the text from response.text() is
redundant; update the error construction inside the response.text().then(...)
callback (the block that creates const error: Error & { _bail?: boolean }) to
test for empty string only (e.g., use text.length > 0 or text !== "" to select
text, otherwise use response.statusText), then set error._bail = bail and reject
as before.
- Around line 152-158: The code is losing the original error stack by creating
new Error instances in the catch of tryOnce; update the bail and timeout/error
branches to preserve the original error as the cause (or rethrow the original
error) instead of throwing plain new Error(reason.message). Specifically, in the
catch handling inside tryOnce's promise you should: for the bail case use throw
new Error("bail: ...", { cause: reason }) or rethrow reason; for the
timeout/AbortError branch ensure you throw TimeoutError or the original reason
with cause preserved (TimeoutError should include the original reason as its
cause if appropriate). Keep references to reason, retries, tries, RetryError,
TimeoutError and tryWith.timedout when making these changes so the original
stack is preserved.

In `@src/errors.ts`:
- Around line 1-20: Change the SDK error hierarchy so all specific errors extend
ABSmartlyError instead of Error: update TimeoutError, RetryError, AbortError
(and any other plain Error subclasses) to subclass ABSmartlyError, ensure their
constructors call super(message) and set this.name (e.g., "TimeoutError",
"RetryError", "AbortError"); keep existing ABSmartlyError, ContextNotReadyError,
and ContextFinalizedError behavior but ensure their prototypes align with
ABSmartlyError so consumers can use instanceof ABSmartlyError for unified error
handling.

In `@src/index.ts`:
- Around line 35-47: The public API currently re-exports types from
"./interfaces" but does not export the DefaultClient class, preventing advanced
users from instantiating or extending it; add an export for DefaultClient from
"./client" (e.g., export { DefaultClient } from "./client") so consumers can
directly import DefaultClient while leaving existing SDKOptions.client injection
unchanged and reference DefaultClient in documentation/examples; locate the
existing export block in this file to add the new export alongside the type
exports.

In `@src/interfaces.ts`:
- Around line 67-74: The ContextPublisher.publish signature currently types the
context parameter as unknown; change it to a more specific type (e.g., Context
or a minimal interface like PublishContext) to improve type safety: update the
publish method signature in the ContextPublisher interface to use that concrete
type for the context parameter, import or declare the Context/PublishContext
type in this file, and then update all implementations of
ContextPublisher.publish to accept and use the new typed context (adjust any
call sites or tests accordingly).

In `@src/jsonexpr/evaluator.ts`:
- Around line 178-181: The boolean branch in the evaluator uses a loose
inequality when checking rvalue from this.booleanConvert(rhs); update the case
"boolean" logic to either use strict comparison (rvalue !== null) or, better,
remove the null check entirely since booleanConvert always returns a boolean;
directly compute and return lhs === rvalue ? 0 : lhs > rvalue ? 1 : -1 in the
case "boolean" block (identify and modify the code around this.booleanConvert
and the local rvalue variable).

In `@src/utils.ts`:
- Around line 107-109: The function arrayEqualsShallow uses a compact
short-circuit: it returns true if both references are identical, otherwise it
checks equal lengths and that no element differs via a?.some((va,vi)=> b && va
!== b[vi]); add a brief clarifying comment above arrayEqualsShallow that
explains these steps (reference identity check, length equality, shallow
strict-equality element comparison) and calls out the optional chaining/guard on
b to avoid null access so readers understand the intent and safety.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7063a39e-9848-4c60-8f14-e9a79111c4e9

📥 Commits

Reviewing files that changed from the base of the PR and between 29fb24a and f6770b1.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (116)
  • .browserslistrc
  • .claude/tasks/context_session_a9973aa3-2e87-420a-9c57-c48fd5f3a2d4.md
  • .editorconfig
  • .eslintignore
  • .eslintrc.js
  • .prettierignore
  • .prettierrc.json
  • README.md
  • babel.config.js
  • docs/superpowers/plans/2026-03-28-typescript-rewrite.md
  • jest.config.js
  • package.json
  • scripts/generate-version.js
  • scripts/generate-version.ts
  • src/__tests__/abort-controller-shim.test.js
  • src/__tests__/algorithm.test.js
  • src/__tests__/algorithm.test.ts
  • src/__tests__/assigner.test.js
  • src/__tests__/assigner.test.ts
  • src/__tests__/client.test.js
  • src/__tests__/client.test.ts
  • src/__tests__/config.test.js
  • src/__tests__/config.test.ts
  • src/__tests__/context.test.js
  • src/__tests__/context.test.ts
  • src/__tests__/errors.test.ts
  • src/__tests__/fetch-shim.test.js
  • src/__tests__/hashing.test.ts
  • src/__tests__/jsonexpr/evaluator.test.js
  • src/__tests__/jsonexpr/evaluator.test.ts
  • src/__tests__/jsonexpr/jsonexpr.test.js
  • src/__tests__/jsonexpr/jsonexpr.test.ts
  • src/__tests__/jsonexpr/operators/and.test.js
  • src/__tests__/jsonexpr/operators/eq.test.js
  • src/__tests__/jsonexpr/operators/evaluator.js
  • src/__tests__/jsonexpr/operators/gt.test.js
  • src/__tests__/jsonexpr/operators/gte.test.js
  • src/__tests__/jsonexpr/operators/in.test.js
  • src/__tests__/jsonexpr/operators/lt.test.js
  • src/__tests__/jsonexpr/operators/lte.test.js
  • src/__tests__/jsonexpr/operators/match.test.js
  • src/__tests__/jsonexpr/operators/not.test.js
  • src/__tests__/jsonexpr/operators/null.test.js
  • src/__tests__/jsonexpr/operators/or.test.js
  • src/__tests__/jsonexpr/operators/semver_eq.test.js
  • src/__tests__/jsonexpr/operators/semver_gt.test.js
  • src/__tests__/jsonexpr/operators/semver_gte.test.js
  • src/__tests__/jsonexpr/operators/semver_lt.test.js
  • src/__tests__/jsonexpr/operators/semver_lte.test.js
  • src/__tests__/jsonexpr/operators/value.test.js
  • src/__tests__/jsonexpr/operators/var.test.js
  • src/__tests__/matcher.test.js
  • src/__tests__/matcher.test.ts
  • src/__tests__/md5.test.js
  • src/__tests__/md5.test.ts
  • src/__tests__/murmur3.test.ts
  • src/__tests__/operators.test.ts
  • src/__tests__/provider.test.js
  • src/__tests__/provider.test.ts
  • src/__tests__/publisher.test.js
  • src/__tests__/publisher.test.ts
  • src/__tests__/sdk.test.js
  • src/__tests__/sdk.test.ts
  • src/__tests__/utils.test.js
  • src/__tests__/utils.test.ts
  • src/abort-controller-shim.ts
  • src/abort.ts
  • src/algorithm.ts
  • src/assigner.ts
  • src/browser.ts
  • src/client.ts
  • src/config.ts
  • src/context.ts
  • src/errors.ts
  • src/fetch-shim.ts
  • src/fetch.ts
  • src/hashing.ts
  • src/index.ts
  • src/interfaces.ts
  • src/jsonexpr/evaluator.ts
  • src/jsonexpr/jsonexpr.ts
  • src/jsonexpr/operators.ts
  • src/jsonexpr/operators/and.ts
  • src/jsonexpr/operators/binary.ts
  • src/jsonexpr/operators/eq.ts
  • src/jsonexpr/operators/gt.ts
  • src/jsonexpr/operators/gte.ts
  • src/jsonexpr/operators/in.ts
  • src/jsonexpr/operators/lt.ts
  • src/jsonexpr/operators/lte.ts
  • src/jsonexpr/operators/match.ts
  • src/jsonexpr/operators/not.ts
  • src/jsonexpr/operators/null.ts
  • src/jsonexpr/operators/or.ts
  • src/jsonexpr/operators/semver_eq.ts
  • src/jsonexpr/operators/semver_gt.ts
  • src/jsonexpr/operators/semver_gte.ts
  • src/jsonexpr/operators/semver_lt.ts
  • src/jsonexpr/operators/semver_lte.ts
  • src/jsonexpr/operators/unary.ts
  • src/jsonexpr/operators/value.ts
  • src/jsonexpr/operators/var.ts
  • src/matcher.ts
  • src/md5.ts
  • src/models.ts
  • src/murmur3.ts
  • src/provider.ts
  • src/publisher.ts
  • src/rfdc.d.ts
  • src/sdk.ts
  • src/utils.ts
  • src/version.ts
  • tsconfig.json
  • tsup.config.ts
  • vitest.config.ts
  • webpack.config.js
💤 Files with no reviewable changes (69)
  • .eslintignore
  • .prettierignore
  • .browserslistrc
  • jest.config.js
  • scripts/generate-version.js
  • .editorconfig
  • src/tests/algorithm.test.js
  • src/tests/assigner.test.js
  • src/tests/jsonexpr/operators/not.test.js
  • src/tests/jsonexpr/operators/null.test.js
  • src/tests/jsonexpr/operators/and.test.js
  • src/tests/jsonexpr/operators/semver_gt.test.js
  • src/tests/jsonexpr/operators/match.test.js
  • .eslintrc.js
  • src/tests/abort-controller-shim.test.js
  • src/jsonexpr/operators/eq.ts
  • src/jsonexpr/operators/or.ts
  • src/jsonexpr/operators/not.ts
  • src/jsonexpr/operators/match.ts
  • src/jsonexpr/operators/semver_gte.ts
  • src/jsonexpr/operators/and.ts
  • src/jsonexpr/operators/semver_eq.ts
  • src/tests/fetch-shim.test.js
  • src/tests/jsonexpr/jsonexpr.test.js
  • src/tests/jsonexpr/operators/in.test.js
  • src/tests/jsonexpr/operators/evaluator.js
  • src/tests/jsonexpr/operators/semver_gte.test.js
  • src/tests/jsonexpr/operators/semver_lt.test.js
  • src/tests/jsonexpr/operators/or.test.js
  • src/tests/jsonexpr/operators/semver_lte.test.js
  • src/tests/jsonexpr/operators/var.test.js
  • src/jsonexpr/operators/in.ts
  • src/jsonexpr/operators/gt.ts
  • src/jsonexpr/operators/gte.ts
  • src/tests/matcher.test.js
  • src/tests/provider.test.js
  • src/tests/md5.test.js
  • src/abort.ts
  • src/browser.ts
  • src/jsonexpr/operators/lte.ts
  • src/jsonexpr/operators/semver_gt.ts
  • src/jsonexpr/operators/value.ts
  • src/jsonexpr/operators/semver_lt.ts
  • src/tests/jsonexpr/operators/lte.test.js
  • src/tests/jsonexpr/operators/gt.test.js
  • src/jsonexpr/operators/var.ts
  • src/jsonexpr/operators/semver_lte.ts
  • src/tests/jsonexpr/operators/semver_eq.test.js
  • src/jsonexpr/operators/unary.ts
  • .prettierrc.json
  • src/jsonexpr/operators/null.ts
  • src/tests/sdk.test.js
  • src/fetch.ts
  • src/tests/utils.test.js
  • src/tests/jsonexpr/evaluator.test.js
  • src/tests/jsonexpr/operators/value.test.js
  • src/tests/client.test.js
  • src/tests/config.test.js
  • src/abort-controller-shim.ts
  • babel.config.js
  • src/tests/publisher.test.js
  • src/fetch-shim.ts
  • src/tests/jsonexpr/operators/eq.test.js
  • src/rfdc.d.ts
  • src/tests/jsonexpr/operators/gte.test.js
  • src/jsonexpr/operators/binary.ts
  • src/jsonexpr/operators/lt.ts
  • webpack.config.js
  • src/tests/jsonexpr/operators/lt.test.js

Comment thread README.md
Comment on lines 181 to +188
```html
<head>
<script type="javascript">
const request = {
units: {
session_id: '5ebf06d8cb5d8137290c4abb64155584fbdb64d8',
},
};

const context = sdk.createContextWith(request, {{ serverSideContext.data() }});
</script>
</head>
<head>
<script type="javascript">
const context = sdk.createContextWith(
{ units: { session_id: "5ebf06d8cb5d8137290c4abb64155584fbdb64d8" } },
{{ serverSideContext.data() }}
);
</script>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

Will an inline HTML <script type="javascript"> execute, or should the type be omitted / set to text/javascript?

💡 Result:

No, an inline HTML <script type="javascript"> will not execute as JavaScript. The type attribute value "javascript" is not a recognized JavaScript MIME type (valid ones include "text/javascript", "application/javascript", or an empty string/no attribute). Browsers treat unrecognized type values as data blocks, not executable scripts. Omit the type attribute entirely or use a valid JavaScript MIME type like type="text/javascript" (or type="application/javascript") for execution. Modern HTML standards encourage omitting the type for classic JavaScript scripts, as it defaults to JavaScript execution.

Citations:


Correct the <script> type attribute in the createContextWith() example.

The type="javascript" attribute is not a recognised JavaScript MIME type. Browsers will treat this script block as inert data and will not execute it. Either omit the type attribute or use type="text/javascript" (or type="application/javascript").

Code snippet
<head>
    <script type="javascript">
        const context = sdk.createContextWith(
            { units: { session_id: "5ebf06d8cb5d8137290c4abb64155584fbdb64d8" } },
            {{ serverSideContext.data() }}
        );
    </script>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 181 - 188, The example in README.md uses an invalid
script type causing the block to be inert; update the <script> tag around the
createContextWith(...) example to either omit the type attribute or use a valid
MIME type such as "text/javascript" (or "application/javascript") so the
JavaScript that calls sdk.createContextWith(...) will execute in the browser.

Comment on lines +3 to +4
const pkg = JSON.parse(readFileSync("package.json", "utf-8"));
writeFileSync("src/version.ts", `export const SDK_VERSION = "${pkg.version}";\n`);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Guard package.json.version before writing generated code.

If version is missing or not a string, the script currently emits an invalid SDK version constant without a clear failure reason.

Suggested fix
-const pkg = JSON.parse(readFileSync("package.json", "utf-8"));
-writeFileSync("src/version.ts", `export const SDK_VERSION = "${pkg.version}";\n`);
+const pkg = JSON.parse(readFileSync("package.json", "utf-8")) as { version?: unknown };
+if (typeof pkg.version !== "string" || pkg.version.length === 0) {
+	throw new Error('Invalid package.json: missing "version" string');
+}
+writeFileSync("src/version.ts", `export const SDK_VERSION = "${pkg.version}";\n`);
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@scripts/generate-version.ts` around lines 3 - 4, Guard the package.json
version before writing src/version.ts: check that the parsed pkg.version exists
and is a string (e.g., validate pkg.version !== undefined && typeof pkg.version
=== "string") in the generate-version script, and if the check fails, print a
clear error and exit non‑zero instead of calling writeFileSync; when valid,
write the SDK_VERSION as before. Ensure you reference pkg.version, the
writeFileSync call that writes `export const SDK_VERSION = ...`, and fail fast
with a descriptive message to avoid emitting an invalid constant.

Comment on lines +90 to +96
beforeEach(() => {
vi.stubGlobal("fetch", vi.fn());
});

afterEach(() => {
vi.restoreAllMocks();
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In Vitest, does vi.restoreAllMocks() restore globals set by vi.stubGlobal(), or must vi.unstubAllGlobals() / test.unstubGlobals be used?

💡 Result:

No, vi.restoreAllMocks does not restore globals set by vi.stubGlobal. It only restores spies created with vi.spyOn or vi.fn by calling .mockRestore on them. Globals stubbed with vi.stubGlobal must be restored using vi.unstubAllGlobals or by enabling the unstubGlobals config option (which calls vi.unstubAllGlobals before each test). test.unstubGlobals is an alias for vi.unstubAllGlobals.

Citations:


🏁 Script executed:

# Check if the test file exists and inspect the specific lines
if [ -f "src/__tests__/client.test.ts" ]; then
  echo "=== Lines 90-96 ==="
  sed -n '90,96p' "src/__tests__/client.test.ts"
  echo ""
  echo "=== Lines 125-131 ==="
  sed -n '125,131p' "src/__tests__/client.test.ts"
  echo ""
  echo "=== Lines 174-182 ==="
  sed -n '174,182p' "src/__tests__/client.test.ts"
else
  echo "Test file not found at src/__tests__/client.test.ts"
fi

Repository: absmartly/javascript-sdk

Length of output: 524


🏁 Script executed:

# Check for vitest configuration files that might set unstubGlobals globally
find . -maxdepth 2 -type f \( -name "vitest.config.*" -o -name "vite.config.*" \) | head -5

Repository: absmartly/javascript-sdk

Length of output: 86


🏁 Script executed:

# Search for any unstubGlobals configuration in the codebase
rg "unstubGlobals|setupFiles|globals:" --type ts --type js --type json -A 2 -B 2 | head -50

Repository: absmartly/javascript-sdk

Length of output: 252


Add vi.unstubAllGlobals() to afterEach blocks to prevent fetch mock leakage across tests.

vi.restoreAllMocks() does not restore globals stubbed via vi.stubGlobal(). Without calling vi.unstubAllGlobals() (or enabling the unstubGlobals config option), mocked globals persist across test suites, breaking test isolation.

Proposed fix
 		afterEach(() => {
 			vi.restoreAllMocks();
+			vi.unstubAllGlobals();
 		});
 		afterEach(() => {
 			vi.useRealTimers();
 			vi.restoreAllMocks();
+			vi.unstubAllGlobals();
 		});

Applies to lines: 90–96, 125–131, 174–182

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
beforeEach(() => {
vi.stubGlobal("fetch", vi.fn());
});
afterEach(() => {
vi.restoreAllMocks();
});
beforeEach(() => {
vi.stubGlobal("fetch", vi.fn());
});
afterEach(() => {
vi.restoreAllMocks();
vi.unstubAllGlobals();
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/client.test.ts` around lines 90 - 96, Several test suites stub
the global fetch via vi.stubGlobal("fetch", vi.fn()) in their beforeEach hooks
but only call vi.restoreAllMocks() in afterEach; add vi.unstubAllGlobals() to
each afterEach that pairs with a vi.stubGlobal call (the afterEach blocks
adjacent to the beforeEach that stubs fetch — look for the
vi.stubGlobal("fetch", vi.fn()) calls in src/__tests__/client.test.ts) so
globals are removed between tests; ensure you call vi.unstubAllGlobals() after
vi.restoreAllMocks() in each affected afterEach (the blocks around lines shown
in the review: the afterEach following the beforeEach that stubs fetch at ~lines
90–96, 125–131, and 174–182).

Comment on lines +61 to +66
test("has correct name and default message", () => {
const error = new AbortError();
expect(error).toBeInstanceOf(Error);
expect(error).toBeInstanceOf(AbortError);
expect(error.name).toBe("AbortError");
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Default-message test does not verify the message.

Line 61 says the test checks the default message, but the assertion is missing. Please assert the message to prevent silent regressions.

Proposed test fix
 test("has correct name and default message", () => {
 	const error = new AbortError();
 	expect(error).toBeInstanceOf(Error);
 	expect(error).toBeInstanceOf(AbortError);
 	expect(error.name).toBe("AbortError");
+	expect(error.message).not.toBe("");
 });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
test("has correct name and default message", () => {
const error = new AbortError();
expect(error).toBeInstanceOf(Error);
expect(error).toBeInstanceOf(AbortError);
expect(error.name).toBe("AbortError");
});
test("has correct name and default message", () => {
const error = new AbortError();
expect(error).toBeInstanceOf(Error);
expect(error).toBeInstanceOf(AbortError);
expect(error.name).toBe("AbortError");
expect(error.message).not.toBe("");
});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/errors.test.ts` around lines 61 - 66, Update the "has correct
name and default message" test for AbortError to actually assert the default
message: after creating const error = new AbortError(); add an assertion that
checks error.message equals the AbortError default (e.g.,
expect(error.message).toBe(AbortError.DEFAULT_MESSAGE) or, if there is no static
constant, assert the known default string used by the class, or at minimum
expect(typeof error.message).toBe("string") and
expect(error.message.length).toBeGreaterThan(0); this ensures the default
message is verified alongside the existing checks for instance and name.

Comment thread src/__tests__/sdk.test.ts
Comment on lines +54 to +62
vi.stubGlobal(
"fetch",
vi.fn().mockResolvedValue({ ok: true, json: () => Promise.resolve({ experiments: [] }) }),
);
const sdk = new SDK(defaultOpts);
const context = sdk.createContext({ units: { session_id: "abc" } });
expect(context).toBeDefined();
vi.restoreAllMocks();
});
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find tests that stub globals and verify whether they also unstub globals.
rg -n "vi\\.stubGlobal|vi\\.unstubAllGlobals|vi\\.restoreAllMocks" src/__tests__ -C2

Repository: absmartly/javascript-sdk

Length of output: 2276


🏁 Script executed:

#!/bin/bash
# Check all test files for stubGlobal patterns
find src/__tests__ -name "*.test.ts" -type f | head -20

Repository: absmartly/javascript-sdk

Length of output: 593


🌐 Web query:

Vitest vi.stubGlobal vi.unstubAllGlobals vi.restoreAllMocks difference

💡 Result:

In Vitest, vi.stubGlobal, vi.unstubAllGlobals, and vi.restoreAllMocks serve distinct purposes in mocking and cleanup: - vi.stubGlobal(name, value): Adds or overrides a global variable on globalThis (or window in jsdom/happy-dom). Unlike direct assignment to globalThis, it tracks the original value for restoration via vi.unstubAllGlobals. Example: vi.stubGlobal('IntersectionObserver', MockClass). Does not auto-reset unless unstubGlobals: true in config or manually called. - vi.unstubAllGlobals: Restores all globals previously stubbed with vi.stubGlobal to their original values (remembers originals on first call). Also handles fake timers (v0.26.0+). Use in afterEach for cleanup. Targets globalThis, global, and browser globals like window. - vi.restoreAllMocks: Restores original implementations for all spies created with vi.spyOn. Clears call history and mock implementations, reverting spied methods/objects. Does not affect vi.stubGlobal, module mocks (vi.mock), or direct assignments. Use for spy cleanup. Key differences: - vi.stubGlobal / vi.unstubAllGlobals pair handles global variable stubbing/restoration. - vi.restoreAllMocks handles spies only. - Use vi.resetAllMocks to reset spies (clears history, sets empty impl) vs restore (original impl). - Always clean up in hooks to prevent test pollution. For complete isolation: afterEach( => { vi.restoreAllMocks; vi.unstubAllGlobals; }).

Citations:


🏁 Script executed:

#!/bin/bash
# Examine sdk.test.ts for all test cases and stubGlobal usage patterns
cat -n src/__tests__/sdk.test.ts | head -80

Repository: absmartly/javascript-sdk

Length of output: 3199


🏁 Script executed:

#!/bin/bash
# Count how many tests in sdk.test.ts stub globals and how many unstub them
echo "=== Tests with stubGlobal ===" && \
rg -c "vi\.stubGlobal" src/__tests__/sdk.test.ts && \
echo "=== Tests with unstubAllGlobals ===" && \
rg -c "vi\.unstubAllGlobals" src/__tests__/sdk.test.ts

Repository: absmartly/javascript-sdk

Length of output: 135


🏁 Script executed:

#!/bin/bash
# Check all test files for stubGlobal patterns and their cleanup
echo "=== Files with stubGlobal ===" && \
fd "\.test\.ts$" src/__tests__ -x rg -l "vi\.stubGlobal" {} && \
echo "" && \
echo "=== Checking each file for unstubAllGlobals ===" && \
for file in $(fd "\.test\.ts$" src/__tests__ -x rg -l "vi\.stubGlobal" {}); do
  echo "File: $file"
  echo "  stubGlobal count: $(rg -c "vi\.stubGlobal" "$file")"
  echo "  unstubAllGlobals count: $(rg -c "vi\.unstubAllGlobals" "$file" 2>/dev/null || echo 0)"
done

Repository: absmartly/javascript-sdk

Length of output: 152


🏁 Script executed:

#!/bin/bash
# Check client.test.ts for stubGlobal and unstubAllGlobals patterns
echo "=== client.test.ts: stubGlobal usage ===" && \
rg -n "vi\.stubGlobal|vi\.unstubAllGlobals" src/__tests__/client.test.ts -B1 -A1

Repository: absmartly/javascript-sdk

Length of output: 338


Add vi.unstubAllGlobals() to properly clean up the stubbed global.

Line 54 stubs global.fetch with vi.stubGlobal(), but line 61 only calls vi.restoreAllMocks(). In Vitest, vi.restoreAllMocks() restores spies only and does not clean up globals stubbed with vi.stubGlobal(). This requires an explicit call to vi.unstubAllGlobals() to prevent test pollution and order-dependent failures.

Suggested fix
 	test("createContext returns Context instance", () => {
 		vi.stubGlobal(
 			"fetch",
 			vi.fn().mockResolvedValue({ ok: true, json: () => Promise.resolve({ experiments: [] }) }),
 		);
 		const sdk = new SDK(defaultOpts);
 		const context = sdk.createContext({ units: { session_id: "abc" } });
 		expect(context).toBeDefined();
+		vi.unstubAllGlobals();
 		vi.restoreAllMocks();
 	});
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/__tests__/sdk.test.ts` around lines 54 - 62, The test stubs the global
fetch via vi.stubGlobal(...) but only calls vi.restoreAllMocks(), so the global
stub remains; update the test to call vi.unstubAllGlobals() after restoring
mocks (or instead of) to fully remove the global stub — locate the SDK
instantiation and context creation in the test (SDK, createContext) and add a
vi.unstubAllGlobals() call (e.g., after vi.restoreAllMocks() or replace it) to
properly clean up the global fetch stub.

Comment thread src/hashing.ts
Comment on lines +7 to +13
if (c >= 0xd800 && c <= 0xdbff && i + 1 < value.length) {
const next = value.charCodeAt(i + 1);
if (next >= 0xdc00 && next <= 0xdfff) {
c = ((c - 0xd800) << 10) + (next - 0xdc00) + 0x10000;
i++;
}
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

In JavaScript (WHATWG Encoding / TextEncoder semantics), how should lone UTF-16 surrogates be encoded in UTF-8, and are raw surrogate code points valid UTF-8 scalar values?

💡 Result:

In JavaScript using WHATWG Encoding / TextEncoder semantics, lone UTF-16 surrogates (code units in the ranges U+D800–U+DBFF or U+DC00–U+DFFF) are not passed directly to the UTF-8 encoder. Instead, when a JavaScript string containing lone surrogates is passed to TextEncoder.encode, it is first converted to a USVString (sequence of Unicode scalar values). During this conversion, any lone surrogates are replaced with the Unicode replacement character U+FFFD (�). The UTF-8 encoder then encodes U+FFFD as the valid 3-byte UTF-8 sequence 0xEF 0xBF 0xBD. Raw surrogate code points (U+D800–U+DFFF) are not valid UTF-8 scalar values. Unicode scalar values exclude surrogates; they are in the range U+0000 to U+10FFFF excluding U+D800–U+DFFF. The WHATWG Encoding Standard defines encoders as operating on scalar values, and standards must ensure no surrogates are present in input queues to UTF-8 encode. Encoding surrogates directly would produce invalid UTF-8 (non-shortest-form 3-byte sequences), which is prohibited.

Citations:


🏁 Script executed:

cat -n src/hashing.ts

Repository: absmartly/javascript-sdk

Length of output: 2757


Replace lone UTF-16 surrogates with the Unicode replacement character (U+FFFD) before UTF-8 encoding.

Lines 7–13 and 18–22 currently allow unpaired UTF-16 surrogates to be encoded directly. This produces invalid UTF-8 byte sequences and violates the WHATWG Encoding standard, which mandates that lone surrogates (U+D800–U+DBFF without a following low surrogate, or any U+DC00–U+DFFF) must be replaced with the replacement character U+FFFD before UTF-8 encoding.

The current logic handles paired surrogates correctly but fails to replace lone surrogates. A lone high surrogate followed by a non-surrogate character, or a lone low surrogate, will be encoded directly, resulting in non-deterministic hashing across runtimes that differ in surrogate handling.

Proposed fix
 export function stringToUint8Array(value: string): Uint8Array {
 	const utf8: number[] = [];
 	for (let i = 0; i < value.length; i++) {
 		let c = value.charCodeAt(i);
-		if (c >= 0xd800 && c <= 0xdbff && i + 1 < value.length) {
-			const next = value.charCodeAt(i + 1);
-			if (next >= 0xdc00 && next <= 0xdfff) {
-				c = ((c - 0xd800) << 10) + (next - 0xdc00) + 0x10000;
-				i++;
-			}
-		}
+		if (c >= 0xd800 && c <= 0xdbff) {
+			if (i + 1 < value.length) {
+				const next = value.charCodeAt(i + 1);
+				if (next >= 0xdc00 && next <= 0xdfff) {
+					c = ((c - 0xd800) << 10) + (next - 0xdc00) + 0x10000;
+					i++;
+				} else {
+					c = 0xfffd;
+				}
+			} else {
+				c = 0xfffd;
+			}
+		} else if (c >= 0xdc00 && c <= 0xdfff) {
+			c = 0xfffd;
+		}
 		if (c < 0x80) {
 			utf8.push(c);
 		} else if (c < 0x800) {
 			utf8.push(0xc0 | (c >> 6), 0x80 | (c & 0x3f));
 		} else if (c < 0x10000) {
 			utf8.push(0xe0 | (c >> 12), 0x80 | ((c >> 6) & 0x3f), 0x80 | (c & 0x3f));
 		} else {
 			utf8.push(0xf0 | (c >> 18), 0x80 | ((c >> 12) & 0x3f), 0x80 | ((c >> 6) & 0x3f), 0x80 | (c & 0x3f));
 		}
 	}
 	return new Uint8Array(utf8);
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (c >= 0xd800 && c <= 0xdbff && i + 1 < value.length) {
const next = value.charCodeAt(i + 1);
if (next >= 0xdc00 && next <= 0xdfff) {
c = ((c - 0xd800) << 10) + (next - 0xdc00) + 0x10000;
i++;
}
}
export function stringToUint8Array(value: string): Uint8Array {
const utf8: number[] = [];
for (let i = 0; i < value.length; i++) {
let c = value.charCodeAt(i);
if (c >= 0xd800 && c <= 0xdbff) {
if (i + 1 < value.length) {
const next = value.charCodeAt(i + 1);
if (next >= 0xdc00 && next <= 0xdfff) {
c = ((c - 0xd800) << 10) + (next - 0xdc00) + 0x10000;
i++;
} else {
c = 0xfffd;
}
} else {
c = 0xfffd;
}
} else if (c >= 0xdc00 && c <= 0xdfff) {
c = 0xfffd;
}
if (c < 0x80) {
utf8.push(c);
} else if (c < 0x800) {
utf8.push(0xc0 | (c >> 6), 0x80 | (c & 0x3f));
} else if (c < 0x10000) {
utf8.push(0xe0 | (c >> 12), 0x80 | ((c >> 6) & 0x3f), 0x80 | (c & 0x3f));
} else {
utf8.push(0xf0 | (c >> 18), 0x80 | ((c >> 12) & 0x3f), 0x80 | ((c >> 6) & 0x3f), 0x80 | (c & 0x3f));
}
}
return new Uint8Array(utf8);
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/hashing.ts` around lines 7 - 13, The surrogate-handling logic allows lone
UTF-16 surrogates to be encoded directly; update the blocks that inspect c,
next, i and value.charCodeAt(...) so that any unpaired high surrogate
(0xD800–0xDBFF with no valid following low surrogate) or any lone low surrogate
(0xDC00–0xDFFF) is replaced with the Unicode replacement code point 0xFFFD
before proceeding to UTF-8 byte emission; specifically, in the branch that
currently computes a paired surrogate (where you set c = ((c - 0xd800) << 10) +
(next - 0xdc00) + 0x10000 and increment i), keep that for valid pairs, but when
the next char is absent or not in 0xDC00–0xDFFF set c = 0xFFFD (and do not
increment i), and in the other branch that sees a low surrogate alone also set c
= 0xFFFD — ensure these changes are applied to the same surrogate-checking
sections so subsequent UTF-8 encoding uses 0xFFFD for lone surrogates.

Comment thread src/interfaces.ts
Comment on lines +15 to +17
export type ContextParams = {
units: Record<string, string | number>;
};
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check how ContextParams is used in Context and SDK

# Search for ContextParams usage to see if attributes are expected
rg -nC3 'ContextParams' --type=ts -g '!*.test.ts'

Repository: absmartly/javascript-sdk

Length of output: 3240


🏁 Script executed:

# Check if there are any comments or documentation about attributes in the PR or codebase
rg -i 'attribute' --type=ts --type=md -g '!*.test.ts' | head -20

Repository: absmartly/javascript-sdk

Length of output: 1108


🏁 Script executed:

# Check the Context class to see how it uses ContextParams internally
sed -n '83,150p' src/context.ts

Repository: absmartly/javascript-sdk

Length of output: 1654


🏁 Script executed:

# Check PublishParams structure and how it relates to attributes
rg -nC5 'PublishParams' --type=ts -g '!*.test.ts' | head -40

Repository: absmartly/javascript-sdk

Length of output: 2005


🏁 Script executed:

# Check if attributes are ever initialized from ContextData in Context
sed -n '1,80p' src/context.ts

Repository: absmartly/javascript-sdk

Length of output: 2681


Add attributes?: Attribute[] to ContextParams for consistency.

The ContextParams type only includes units, but the codebase fully supports attributes through the Attribute interface and PublishParams. Currently, attributes must be added after context creation via method calls, whereas units can be initialised directly via ContextParams. For API consistency, add an optional attributes property to allow initial attribute population during context instantiation.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/interfaces.ts` around lines 15 - 17, ContextParams currently only exposes
units, but the codebase supports initial attributes via the Attribute interface
and PublishParams; add an optional attributes?: Attribute[] to the ContextParams
type so callers can supply initial attributes on creation, update any
constructors or factory functions that accept ContextParams (e.g., context
creation methods) to read and apply params.attributes when present, and ensure
typings referencing ContextParams (and any usage in PublishParams flows) accept
and propagate the new optional field.

Comment thread src/jsonexpr/operators.ts
Comment on lines +31 to +37
export class OrCombinator {
evaluate(evaluator: Evaluator, args: unknown): boolean | null {
if (Array.isArray(args)) {
for (const expr of args) {
if (evaluator.booleanConvert(evaluator.evaluate(expr))) return true;
}
return args.length === 0;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Return false for an empty or expression.

args.length === 0 currently makes { or: [] } truthy, which flips the normal identity for disjunction and can turn an empty clause into a universal match.

🛠️ Proposed fix
 export class OrCombinator {
 	evaluate(evaluator: Evaluator, args: unknown): boolean | null {
 		if (Array.isArray(args)) {
 			for (const expr of args) {
 				if (evaluator.booleanConvert(evaluator.evaluate(expr))) return true;
 			}
-			return args.length === 0;
+			return false;
 		}
 		return null;
 	}
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/jsonexpr/operators.ts` around lines 31 - 37, The OrCombinator.evaluate
implementation currently treats an empty args array as truthy by returning
args.length === 0; change this so that an empty disjunction returns false
instead. Locate the OrCombinator.evaluate method in src/jsonexpr/operators.ts
and replace the final return that checks args.length with a straight false (so {
or: [] } evaluates to false) while keeping the existing loop and
evaluator.booleanConvert(evaluator.evaluate(expr)) logic intact.

Comment thread src/sdk.ts
Comment on lines 60 to +72
constructor(options: ClientOptions & SDKOptions) {
const clientOptions = Object.assign(
{
agent: "absmartly-javascript-sdk",
},
...Object.entries(options || {})
.filter(
(x) =>
["application", "agent", "apiKey", "endpoint", "keepalive", "environment", "retries", "timeout"].indexOf(
x[0]
) !== -1
)
.map((x) => ({ [x[0]]: x[1] }))
);

options = Object.assign({}, options);

this._client = options.client || new Client(clientOptions);
if (options.client) {
this._client = options.client;
} else {
const clientOptions = Object.assign(
{ agent: "absmartly-javascript-sdk" },
...Object.entries(options || {})
.filter((x) => CLIENT_OPTION_KEYS.indexOf(x[0]) !== -1)
.map((x) => ({ [x[0]]: x[1] })),
) as ClientOptions;

this._client = new DefaultClient(clientOptions);
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n 'constructor\(options: .*ClientOptions.*SDKOptions' src/sdk.ts
rg -n 'type ClientOptions =|type SDKOptions =' src/interfaces.ts
rg -n -A3 -B1 'client: new MyCustomClient' README.md

Repository: absmartly/javascript-sdk

Length of output: 344


🏁 Script executed:

sed -n '35,70p' src/interfaces.ts

Repository: absmartly/javascript-sdk

Length of output: 998


Adjust constructor typing to support dependency injection without requiring DefaultClient options.

The current signature constructor(options: ClientOptions & SDKOptions) requires apiKey, application, endpoint, and environment even when options.client is provided. This contradicts the documented usage pattern shown in README.md (lines 407-410), where new SDK({ client: new MyCustomClient() }) should work without these fields.

TypeScript consumers following the documentation will encounter type errors. The suggested union type makes ClientOptions fields partial when a custom client is supplied, aligning the type signature with the intended API.

💡 Suggested typing shape
+type SDKConstructorOptions =
+	| (ClientOptions & SDKOptions & { client?: undefined })
+	| ({ client: Client } & Omit<SDKOptions, "client"> & Partial<ClientOptions>);
+
 export class SDK {
 	// ...
-	constructor(options: ClientOptions & SDKOptions) {
+	constructor(options: SDKConstructorOptions) {
 		if (options.client) {
 			this._client = options.client;
 		} else {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/sdk.ts` around lines 60 - 72, The constructor currently typed as
constructor(options: ClientOptions & SDKOptions) forces required ClientOptions
even when options.client is provided; change the signature to a union that
allows providing a custom client without supplying DefaultClient fields, e.g.
constructor(options: (ClientOptions & SDKOptions) | (Partial<ClientOptions> &
SDKOptions & { client: ClientInterface })), leaving the internal logic using
options.client and new DefaultClient(clientOptions) unchanged; update any
related type imports (ClientOptions, SDKOptions, ClientInterface) and ensure
this._client assignment code still compiles against the new union type.

Comment thread src/sdk.ts
Comment on lines +139 to +150
private static _validateParams(params: ContextParams): void {
for (const [key, value] of Object.entries(params.units)) {
const type = typeof value;
if (type !== "string" && type !== "number") {
throw new Error(
`Unit '${entry[0]}' UID is of unsupported type '${type}'. UID must be one of ['string', 'number']`
`Unit '${key}' UID is of unsupported type '${type}'. UID must be one of ['string', 'number']`,
);
}

if (typeof entry[1] === "string") {
if (entry[1].length === 0) {
throw new Error(`Unit '${entry[0]}' UID length must be >= 1`);
}
if (typeof value === "string" && value.length === 0) {
throw new Error(`Unit '${key}' UID length must be >= 1`);
}
});
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Handle missing units explicitly before validation.

Context already treats params.units as optional in src/context.ts Lines 102-104, but _validateParams() unconditionally iterates it. createContext({}) and createContextWith({}, data) will therefore throw a native TypeError instead of the intended validation behaviour.

🛠️ Proposed fix
 private static _validateParams(params: ContextParams): void {
+	if (!params.units) return;
 	for (const [key, value] of Object.entries(params.units)) {
 		const type = typeof value;
 		if (type !== "string" && type !== "number") {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private static _validateParams(params: ContextParams): void {
for (const [key, value] of Object.entries(params.units)) {
const type = typeof value;
if (type !== "string" && type !== "number") {
throw new Error(
`Unit '${entry[0]}' UID is of unsupported type '${type}'. UID must be one of ['string', 'number']`
`Unit '${key}' UID is of unsupported type '${type}'. UID must be one of ['string', 'number']`,
);
}
if (typeof entry[1] === "string") {
if (entry[1].length === 0) {
throw new Error(`Unit '${entry[0]}' UID length must be >= 1`);
}
if (typeof value === "string" && value.length === 0) {
throw new Error(`Unit '${key}' UID length must be >= 1`);
}
});
}
private static _validateParams(params: ContextParams): void {
if (!params.units) return;
for (const [key, value] of Object.entries(params.units)) {
const type = typeof value;
if (type !== "string" && type !== "number") {
throw new Error(
`Unit '${key}' UID is of unsupported type '${type}'. UID must be one of ['string', 'number']`,
);
}
if (typeof value === "string" && value.length === 0) {
throw new Error(`Unit '${key}' UID length must be >= 1`);
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/sdk.ts` around lines 139 - 150, The _validateParams method currently
assumes params.units exists and iterates it, causing a TypeError when callers
like createContext or createContextWith pass no units; update _validateParams to
explicitly handle a missing or falsy params.units (e.g., treat undefined/null as
an empty object or return early) before the for-loop so validation only runs
when units is present, keeping the existing checks for type and empty strings
intact and referencing _validateParams, createContext, and createContextWith to
locate the related call sites.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant